Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replication: define error conditions for GetVolumeReplicationInfo #47

Merged
merged 1 commit into from
Nov 1, 2022

Conversation

Madhu-1
Copy link
Member

@Madhu-1 Madhu-1 commented Oct 28, 2022

In some cases the driver might not be able to return the Replication details for some time. In that case, it can return well-known error codes based on that CO can retry to call the GetVolumeReplicationInfo in exponential backoff to get the required details.

Signed-off-by: Madhu Rajanna madhupr007@gmail.com

@mergify mergify bot added the design Adds or updates an operation or service label Oct 28, 2022
@Madhu-1 Madhu-1 requested a review from yati1998 October 28, 2022 09:20
@ShyamsundarR
Copy link
Contributor

Making this optional is fine, but when lastSyncTime is not present in status, what is the assumption consumers would have in terms of the time it was last synced? IOW, if a driver does not support this then this cannot be used. A driver that supports this, may not update it at which point there needs to be an assumption made by consumers that this is not yet synced. How would consumers know the capability and act accordingly is the key question.

@Madhu-1
Copy link
Member Author

Madhu-1 commented Oct 28, 2022

Making this optional is fine, but when lastSyncTime is not present in status, what is the assumption consumers would have in terms of the time it was last synced? IOW, if a driver does not support this then this cannot be used. A driver that supports this, may not update it at which point there needs to be an assumption made by consumers that this is not yet synced. How would consumers know the capability and act accordingly is the key question.

When the last sync time is not present in the current request, the consumer should reuse the last known time for previous RPC calls.

@ShyamsundarR
Copy link
Contributor

Making this optional is fine, but when lastSyncTime is not present in status, what is the assumption consumers would have in terms of the time it was last synced? IOW, if a driver does not support this then this cannot be used. A driver that supports this, may not update it at which point there needs to be an assumption made by consumers that this is not yet synced. How would consumers know the capability and act accordingly is the key question.

When the last sync time is not present in the current request, the consumer should reuse the last known time for previous RPC calls.

Which is fine, question is when a driver does not support this there is no prior value as well, and how does the consumer know and communicate to the user or other consumers that this is not supported?

@Madhu-1
Copy link
Member Author

Madhu-1 commented Oct 28, 2022

Which is fine, question is when a driver does not support this there is no prior value as well, and how does the consumer know and communicate to the user or other consumers that this is not supported?

In that case, no LastSyncTime will be added and it's based on the capability.

  • The consumer first checks whether the driver supports the GetReplicationInfo API call, if yes use lastSyncTime, if the lastSyncTime is not present in the RPC call use last known LastSyncTime
  • If the capability itself is not supported by the Driver, dont make RPC call nor advertise lastSyncTime to others.

@nixpanic
Copy link
Contributor

If a driver does not ever return the optional LastSyncTime in the GetReplicationInfo response. What are consumers expected to use as LastSyncTime?

@Madhu-1
Copy link
Member Author

Madhu-1 commented Oct 31, 2022

If a driver does not ever return the optional LastSyncTime in the GetReplicationInfo response. What are consumers expected to use as LastSyncTime?

If the driver is not returning, user will use last known LastSyncTime

@nixpanic
Copy link
Contributor

If a driver does not ever return the optional LastSyncTime in the GetReplicationInfo response. What are consumers expected to use as LastSyncTime?

If the driver is not returning, user will use last known LastSyncTime

I still must be missing something... GetVolumeReplicationInfo is the only place where LastSyncTime could be returned. There is no other way to find out when the syncing was last finished, I think? Consumers may not rely on optional attributes for their functioning, so the LastSyncTime in GetVolumeReplicationInfo can only be used informative?

@Madhu-1
Copy link
Member Author

Madhu-1 commented Oct 31, 2022

If a driver does not ever return the optional LastSyncTime in the GetReplicationInfo response. What are consumers expected to use as LastSyncTime?

If the driver is not returning, user will use last known LastSyncTime

I still must be missing something... GetVolumeReplicationInfo is the only place where LastSyncTime could be returned. There is no other way to find out when the syncing was last finished, I think? Consumers may not rely on optional attributes for their functioning, so the LastSyncTime in GetVolumeReplicationInfo can only be used informative?

Yes, its for information only the consumer cannot decide based on it.

There is no other way to find out when the syncing was last finished

The final sync complete is based on the readyToUse flag in the resync RPC call.

@nixpanic
Copy link
Contributor

I don't see it being useful for a driver to implement GetVolumeReplicationInfo, but unable to return the last_sync_time. At the moment it is the only attribute of the response. Returning the "last known LastSyncTime" seems wrong, it may not always be the correct last time if some error occurred, better to return an error in that case and retry later.

I'd like to see a description of errors that can be returned on a GetVolumeReplicationInfo call, with notes that some errors can be retried later, whereas others are final failures and should not be retried.

Keeping the last_sync_time REQUIRED makes it clear to driver implementers and consumers of the API what the expectations are.

@Madhu-1 Madhu-1 changed the title replication: make last_sync_time as optional replication: define error conditions for GetVolumeReplicationInfo Oct 31, 2022
@Madhu-1
Copy link
Member Author

Madhu-1 commented Oct 31, 2022

@nixpanic PTAL

@Madhu-1 Madhu-1 force-pushed the make-last-time-optional branch 2 times, most recently from 63ccc36 to 2f3cc66 Compare October 31, 2022 14:59
In some cases the driver might not be able to return the
Replication details for a period of time, in that case
it can return well known error codes based on that CO
can retry to call the GetVolumeReplicationInfo in exponential
backoff to get the required details.

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
Copy link
Contributor

@nixpanic nixpanic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! keeping the last_sync_time as required and defined errors is much better 👍

@mergify mergify bot merged commit 98eff76 into csi-addons:main Nov 1, 2022
Madhu-1 added a commit to Madhu-1/ceph-csi that referenced this pull request Nov 1, 2022
csi-addons/spec#47
defines the error messages for the
GetVolumeReplicationInfo RPC call.

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
Madhu-1 added a commit to Madhu-1/kubernetes-csi-addons that referenced this pull request Nov 1, 2022
csi-addons/spec#47
has the defined errors for the
GetVolumeReplicationInfo RPC call.

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
Madhu-1 added a commit to Madhu-1/ceph-csi that referenced this pull request Nov 1, 2022
csi-addons/spec#47
defines the error messages for the
GetVolumeReplicationInfo RPC call.

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
Madhu-1 added a commit to Madhu-1/ceph-csi that referenced this pull request Nov 1, 2022
csi-addons/spec#47
defines the error messages for the
GetVolumeReplicationInfo RPC call.

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
Madhu-1 added a commit to Madhu-1/ceph-csi that referenced this pull request Nov 2, 2022
csi-addons/spec#47
defines the error messages for the
GetVolumeReplicationInfo RPC call.

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
Madhu-1 added a commit to Madhu-1/kubernetes-csi-addons that referenced this pull request Nov 2, 2022
csi-addons/spec#47
has the defined errors for the
GetVolumeReplicationInfo RPC call.

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
Madhu-1 added a commit to Madhu-1/ceph-csi that referenced this pull request Nov 2, 2022
csi-addons/spec#47
defines the error messages for the
GetVolumeReplicationInfo RPC call.

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
mergify bot pushed a commit to csi-addons/kubernetes-csi-addons that referenced this pull request Nov 2, 2022
csi-addons/spec#47
has the defined errors for the
GetVolumeReplicationInfo RPC call.

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
mergify bot pushed a commit to ceph/ceph-csi that referenced this pull request Nov 3, 2022
csi-addons/spec#47
defines the error messages for the
GetVolumeReplicationInfo RPC call.

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
Madhu-1 added a commit to Madhu-1/ceph-csi that referenced this pull request Nov 3, 2022
csi-addons/spec#47
defines the error messages for the
GetVolumeReplicationInfo RPC call.

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
(cherry picked from commit 3cce629)
Madhu-1 added a commit to Madhu-1/kubernetes-csi-addons that referenced this pull request Nov 3, 2022
csi-addons/spec#47
has the defined errors for the
GetVolumeReplicationInfo RPC call.

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
(cherry picked from commit 09674b2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design Adds or updates an operation or service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants